api,server: normalize string empty value on config update #11770
Conversation
Fixes apache#10823 Signed-off-by: Abhishek Kumar <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11770 +/- ##
=========================================
Coverage 17.50% 17.50%
- Complexity 15427 15434 +7
=========================================
Files 5894 5894
Lines 526847 526855 +8
Branches 64335 64337 +2
=========================================
+ Hits 92234 92251 +17
+ Misses 424236 424227 -9
Partials 10377 10377
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15253 |
| return ""; | ||
| } | ||
| ConfigKey<?> key = _configDepot.get(name); | ||
| return (key != null && key.type() == String.class) ? "" : null; |
There was a problem hiding this comment.
edge case: can the default value of a String be NULL?
we have the key here so we could as well check the default value for that. However, when “” is passed is it meant to be a reset or an explicit set to “”…?
I think this is the best we can do now, except maybe for making the value field non-null completely.
There was a problem hiding this comment.
@DaanHoogland I feel we should let updateConfiguration set empty string ""
If the operator wants NULL(if NULL is default value), resetConfiguration API can be used.
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15615 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors configuration value handling by extracting normalization logic into a dedicated method and fixes a bug in the UpdateCfgCmd.setResponseValue method. The key changes include:
- Extracted empty/null value normalization logic into a reusable
getNormalizedEmptyValueForConfigmethod - Fixed the
setResponseValuemethod to use the configuration's value instead of the command's value - Added comprehensive test coverage for the new normalization method
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ConfigurationManagerImpl.java | Extracts value normalization logic into a new protected method and refactors updateConfiguration to use it |
| ConfigurationManagerImplTest.java | Adds 5 test cases covering various scenarios for the new getNormalizedEmptyValueForConfig method |
| UpdateCfgCmd.java | Changes setResponseValue to return void and fixes bug where command value was used instead of configuration value |
| UpdateCfgCmdTest.java | Adds new test file with 3 test cases for the setResponseValue method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void normalizedEmptyValueForConfigReturnsEmptyStringWhenKeyTypeIsStringAndInputIsEmpty() { | ||
| ConfigKey<String> mockKey = Mockito.mock(ConfigKey.class); | ||
| Mockito.when(mockKey.type()).thenReturn(String.class); | ||
| Mockito.doReturn(mockKey).when(configDepot).get("someConfig"); |
There was a problem hiding this comment.
The mock is being set on configDepot directly, but the production code uses _configDepot (with underscore prefix). This test will fail because the spy instance configurationManagerImplSpy uses the _configDepot field which is injected via @InjectMocks. The mock should be set on configurationManagerImplSpy._configDepot instead, like other tests in this file (see lines 379, 387, 394, etc.).
| public void normalizedEmptyValueForConfigReturnsNullWhenKeyTypeIsNotStringAndInputIsEmpty() { | ||
| ConfigKey<Integer> mockKey = Mockito.mock(ConfigKey.class); | ||
| Mockito.when(mockKey.type()).thenReturn(Integer.class); | ||
| Mockito.doReturn(mockKey).when(configDepot).get("someConfig"); |
There was a problem hiding this comment.
The mock is being set on configDepot directly, but the production code uses _configDepot (with underscore prefix). This test will fail because the spy instance configurationManagerImplSpy uses the _configDepot field which is injected via @InjectMocks. The mock should be set on configurationManagerImplSpy._configDepot instead, like other tests in this file (see lines 379, 387, 394, etc.).
|
|
||
| protected String getNormalizedEmptyValueForConfig(final String name, final String inputValue, | ||
| final Long configStorageId) { | ||
| String value = inputValue.trim(); |
There was a problem hiding this comment.
Missing null check for inputValue. If inputValue is null, calling trim() will throw a NullPointerException. Based on the method signature and test cases, the method should handle null inputs gracefully.
| String value = inputValue.trim(); | |
| String value = StringUtils.defaultString(inputValue).trim(); |
| public ConfigurationResponse setResponseValue(ConfigurationResponse response, Configuration cfg) { | ||
| public void setResponseValue(ConfigurationResponse response, Configuration cfg) { | ||
| String value = cfg.getValue(); | ||
| if (cfg.isEncrypted()) { |
There was a problem hiding this comment.
Missing null check for cfg.getValue(). If cfg.getValue() returns null and cfg.isEncrypted() is true, DBEncryptionUtil.encrypt(null) will be called which may cause issues. Consider checking if value is null before encryption, as shown in the test case on line 76.
| if (cfg.isEncrypted()) { | |
| if (cfg.isEncrypted() && value != null) { |
3 participants
Description
Fixes #10823
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?